Fix/declarative config no exceptions#8079
Fix/declarative config no exceptions#8079Bhagirath00 wants to merge 4 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8079 +/- ##
============================================
+ Coverage 90.20% 90.34% +0.13%
+ Complexity 7592 7591 -1
============================================
Files 841 839 -2
Lines 22911 22838 -73
Branches 2288 2280 -8
============================================
- Hits 20666 20632 -34
+ Misses 1529 1498 -31
+ Partials 716 708 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java
Outdated
Show resolved
Hide resolved
api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java
Outdated
Show resolved
Hide resolved
api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java
Outdated
Show resolved
Hide resolved
api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentSetter.java
Outdated
Show resolved
Hide resolved
...io/opentelemetry/sdk/extension/incubator/fileconfig/YamlDeclarativeConfigPropertiesTest.java
Show resolved
Hide resolved
...io/opentelemetry/sdk/extension/incubator/fileconfig/YamlDeclarativeConfigPropertiesTest.java
Show resolved
Hide resolved
No need to bloat the PR description with this type of data which the build checks already tell us ;) |
43cad9b to
6eed41c
Compare
|
Docs updates look good. I agree with @robsunday that the EnvironmentGetter/Setter don't need to be in this PR. |
|
@robsunday @jack-berg |
robsunday
left a comment
There was a problem hiding this comment.
I still have some minor comments, but I approve this PR
| @Nullable | ||
| @Override | ||
| public String getString(String name) { | ||
| Objects.requireNonNull(name, "name"); |
There was a problem hiding this comment.
The only doubt I have here is that the result of getString(null) will be NPE with quite cryptic message "name".
Maybe the message should be a bit more descriptive? Like "Null configuration property name".
This applies to all changes below.
There was a problem hiding this comment.
Good catch, updated the message to be more descriptive. Thanks!
| * @throws DeclarativeConfigException if the property is not a valid scalar string | ||
| * {@code name} has not been configured or is not a valid scalar string | ||
| */ | ||
| default String getString(String name, String defaultValue) { |
There was a problem hiding this comment.
For consistency I'd recommend adding here also:
Objects.requireNonNull(defaultValue, "Null default value");
There was a problem hiding this comment.
Makes sense, added the null check for defaultValue too.
There was a problem hiding this comment.
We need to get consistent about whether or not we do this. We rely on javax.annotation.Nullable annotation, the assumption that params and returns are non-null unless otherwise specified, leveraging the net.ltgt.nullaway error prone plugin to validate at build time. Of course, at runtime there is no guarantee that callers adhere to these annotations, and so we have code like this which checks for null despite the params being non-null according to the annotations.
We need to be consistent about:
- When and why do we add additional null checks
- Where do we do this (i.e. in interface default methods or in the actual implementation)
- What is the behavior when the null annotation is violated (throw or noop)
Let's hold off here until we get some repo level guidance on this. ConfigProperties, the env var / sys property analog of DeclarativeConfigProperties, has the exact same method and does not do this null check.
|
@robsunday Pushed the fixes based on your feedback. |
|
@robsunday @jack-berg Plz review it. |
|
Any Update?? |
|
cc @open-telemetry/java-approvers please can you take a look? |
|
@jack-berg plz look to this |
jack-berg
left a comment
There was a problem hiding this comment.
If I understand correctly, the implementation was already not throwing when a property had a type mismatch. This just updates the javadoc and tests to reflect that.
issue: #7929
Description:
This PR updates the
DeclarativeConfigPropertiesAPI to returnnullinstead of throwing aDeclarativeConfigExceptionwhen a property exists but has a different data type than requested (e.g., calling getBoolean on a String value).Changes:
In
DeclarativeConfigProperties.java(API): Updated Javadocs for all getter methods to explicitly state that they return null on type mismatch.In
YamlDeclarativeConfigPropertiesTest.java(Tests): Added comprehensive verification tests to ensure thatgetString,getBoolean,getInt,getStructured, andgetStructuredListall return null correctly when a type mismatch occurs.Verification Results:
All tests in the incubator module passed successfully: